Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn on literal patterns found anywhere in a constructor's arguments. #2133

Merged
merged 5 commits into from
Nov 7, 2018

Conversation

yallop
Copy link
Member

@yallop yallop commented Nov 3, 2018

The attribute @ocaml.warn_on_literal_pattern enables a warning for code that uses a literal pattern to match a constructor's arguments.

# exception E of string [@ocaml.warn_on_literal_pattern];;
exception E of string
# try () with E "foo" -> ();;
Characters 14-19:
  try () with E "foo" -> ();;
                ^^^^^
Warning 52: Code should not depend on the actual values of
this constructor's arguments. They are only for information
and may change in future versions. (See manual section 9.5)
- : unit = ()

Predefined exceptions, such as Failure, are marked with the attribute to dissuade people writing exception-handling code from relying on the value of the exception's arguments:

# try () with Failure "foo" -> ();;
Line 1, characters 20-25:
1 | try () with Failure "foo" -> ();;
                        ^^^^^
Warning 52: Code should not depend on the actual values of
this constructor's arguments. They are only for information
and may change in future versions. (See manual section 9.5)
- : unit = ()

However, the warning is currently arbitrarily restricted to constructors of a single non-tuple argument, and so no warning is emitted for more complex patterns:

# try "" with Failure ("foo" as s) -> s;;
- : string = ""
# try () with Match_failure ("foo", _, _) -> ();;
- : unit = ()
# try () with Failure ("foo" : string ) -> ();;
- : unit = ()

This PR extends the definition of 'literal pattern' so that warnings are emitted in these cases, too.

#254 has some previous discussion of the attribute/warning, and the possibility of extending them beyond a single constructor. @gasche's comment is particularly relevant:

#Following #379, I have regrets about the current state of the this PR. Due to implementation limitations, we only support constructors with a single argument, but using the attribute on constructors with several arguments will just silently make it meaningless and not warn the user in any way. This is very bad usability.

Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

I think that the behavior with the PR is better than without. The design is still not fully satisfying however: having the attribute on the constructor (rather than on a parameter or possibly sub-types of the parameter) forces a very coarse-grained behavior, with some risk of users being unable to express their intent on which part they would like to avoid warnings about.

Could you maybe add a couple examples test-cases that did not warn before and now correctly warn with the PR?

typing/typecore.ml Outdated Show resolved Hide resolved
typing/typecore.ml Outdated Show resolved Hide resolved
@yallop
Copy link
Member Author

yallop commented Nov 5, 2018

@gasche:

Could you maybe add a couple examples test-cases that did not warn before and now correctly warn with the PR?

Good idea. 3d18694 adds a few such cases.

Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve of the PR, but:

  • you must add a Changes entry (and possibly check in the description of the warning in the manual whether it needs an update), and

  • I think that the testsuite file that you touched would be much nicer with expect-style test, it would be nice if you updated it to this style. (I think it's a general good-hygiene principle to update each file we touch to gradually migrate.)

@yallop
Copy link
Member Author

yallop commented Nov 5, 2018

@gasche:

you must add a Changes entry

Done (5f14aa1).

(and possibly check in the description of the warning in the manual whether it needs an update)

Also done (894df10). The existing text is mostly okay, but I added a short paragraph to make it clear that the warning applies where the pattern is not a literal itself, but contains literals as sub-patterns.

I think that the testsuite file that you touched would be much nicer with expect-style test, it would be nice if you updated it to this style.

Also done (5521787).

@gasche
Copy link
Member

gasche commented Nov 5, 2018

Thanks! This is good to go if/when CI passes. If I were to forget to come check it again, please (anyone) feel free to ping or merge.

@yallop
Copy link
Member Author

yallop commented Nov 7, 2018

Thanks, @gasche. CI is passing.

@gasche gasche merged commit d5e75fb into ocaml:trunk Nov 7, 2018
@yallop yallop deleted the warn-on-literal-full branch November 19, 2018 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants